Constify remaining traits/impls for `const_ops` #143949

clarfonthey · 2025-07-15T01:03:23Z

Tracking issue: #143802

This is split into three commits for ease of reviewability:

Updates the forward_ref_* macros to accept multiple attributes (in anticipation of needing rust_const_unstable attributes) and also require attributes in these macros. Since the default attribute only helps for the initial implementations, it means it's easy to get wrong for future implementations, as shown for the saturating implementations which were incorrect before.
Adds constness and trivial implementations to the remaining traits in core::ops::{arith,bits}.
Constifies the rest of the reasonable implementations.

A few random other notes on the implementation specifically:

Unfortunately, without Tracking Issue for macro_named_capture_groups #142999 implemented, we can't make the constness in the forward_ref_* macros work nicely without copy-pasting the macro body. Instead of doing that, I decided to go for a cursed extra const argument, which is then used for the actual const keyword. I explicitly mention named macro capture groups as a way of doing this better in the comments.
I unindented the attributes that were passed to the forward_ref_* macro calls because in some places rustfmt wanted them to be unindented, and in others it was allowed because they were themselves inside of macro bodies. I chose the consistent indenting even though I (personally) think it looks worse.

As far as the actual changes go, this constifies the following additional traits:

Neg
Not
BitAnd
BitOr
BitXor
Shl
Shr
AddAssign
SubAssign
MulAssign
DivAssign
RemAssign
BitAndAssign
BitOrAssign
BitXorAssign
ShlAssign
ShrAssign

In terms of constified implementations of these traits, it adds the reference-forwarded versions of all the arithmetic operators, which are defined by the macros in library/core/src/internal_macros.rs. I'm not going to fully enumerate these because we'd be here all day, but sufficed to say, it effectively allows adding an & to one or both sides of an operator for primitives.

Additionally, I constified the implementations for Wrapping and Saturating as well, since all of them forward to already-const-stable methods.

There are three "non-primitive" types which implement these traits, listed below. Note that I put "non-primitive" in quotes since I'm including Wrapping and Saturating, which are just wrappers over primitives.

Duration (arithmetic operations)
Ipv4Addr (bit operations)
Ipv6Addr (bit operations)

Note

Concerns (0 active)

~~May break Clippy~~ resolved in this comment

Managed by @rustbot—see help for details.

rustbot · 2025-07-15T01:03:27Z

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-15T01:03:31Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

hkBst · 2025-07-15T19:23:43Z

src/tools/clippy/tests/ui/assign_ops.rs

@@ -75,6 +76,7 @@ impl MulAssign<i64> for Wrap {
    }
 }

+#[clippy::msrv = "1.88.0"]


What is this for?

This hides a real issue: to know whether it can replace "a = a OP b" by "a OP= b" (where OP is, for example, "+"), Clippy looks for the const-stability of the sole associated item of the trait corresponding to "OP=". Here, AddAssign is now identified as const-stable, and Clippy attempts to replace a = a + b; by a += b; in a const context, when a is of a generic type implementing AddAssign.

Yes: I'm not quite sure whether this code should even remain on Clippy's side after this PR, since I'm pretty sure that "MSRV" doesn't account for nightly compilers, and we definitely won't want to ever stabilise an operator without its corresponding assign version. I chose the easiest approach to get it to pass without making too much of a modification, since it makes more sense to edit this in the clippy repo directly instead of via the subtree.

Filed rust-lang/rust-clippy#15285 to keep track of this.

In the meantime, you could go with https://rfc1149.pastes.sh/patch-assign-op-pattern.diff and remove the msrv attribute, and we'll take care of reintroducing the proper code in Clippy.

In the test, res = res + Self::ONE; cannot be replaced by res += Self::ONE in a const context because T (the type of res and Self::ONE) implements AddAssign, not [const] AddAssign (and it implements [const] Add, hence the valid + operation in the const context). But Clippy, if it stays unpatched, will suggest it because it only checks the constness of the AddAssign trait declaration, not its implementation.

If I am mistaken and this suggestion is valid, then instead of commenting the test, just add a //~^ assign_op_pattern below the res = res + Self::ONE; line and bless the results, but I think you'll get an invalid .fixed file.

Oh, you're totally correct here; I was mistaken. So, this is a genuine bug, but it's unrelated to the actual issue at hand, IMHO: it should also suggest to replace [const] Add with [const] AddAssign, or add it. Or at least make it not an automatically applicable thing. It has nothing to do with the const stability of the trait, but the const bound on the trait.

Indeed, this is a genuine bug in Clippy, but it activates only when your PR goes in, unless you apply something similar to the patch at https://rfc1149.pastes.sh/patch-assign-op-pattern.diff which neutralizes the bug.

If you prefer, I can submit this patch as a PR to the current repository to neutralize the bug before the current PR gets merged, but this will be no better than integrating it as part of this PR. I don't suggest to submit it to the Clippy repo, as it won't get merged into the compiler repo before the next sync on July 24.

That's fair, and now that I understand, I'll go with your patch instead.

library/core/src/ops/arith.rs

rustbot · 2025-07-16T22:03:02Z

Only organization members can add or resolve concerns.

samueltardieu · 2025-07-16T22:27:11Z

@rustbot concern May break Clippy

(checking if concerns work as top-level comments only, real concern was written here, and discussion about the error message is on Zulip)

samueltardieu · 2025-07-17T11:07:33Z

@rustbot resolve May break Clippy

We cleared up the misunderstanding of what the problem was.

clarfonthey · 2025-07-17T12:46:08Z

Yeah, I'll be honest, I have genuinely no idea what those tests are doing. I could just add the feature flag, but it wasn't necessary before, and I don't know why.

samueltardieu · 2025-07-17T13:16:08Z

Yeah, I'll be honest, I have genuinely no idea what those tests are doing. I could just add the feature flag, but it wasn't necessary before, and I don't know why.

Didn't you just forget to replace #![feature(const_ops)] in library/coretests/tests/lib.rs?

samueltardieu · 2025-07-17T13:19:30Z

src/tools/clippy/tests/ui/assign_ops.rs

+#![feature(const_arith_ops)]
+#![feature(const_trait_impl)]


If you keep both into a single #![feature] inner attribute, then it won't make a difference in line numbers in the stderr files (after you're reblessed the tests).

I know, but I don't like compromising formatting just because of the line number changes.

clarfonthey · 2025-07-18T00:00:06Z

Didn't you just forget to replace #![feature(const_ops)] in library/coretests/tests/lib.rs?

Last time I checked this file, it didn't have it, but now it does. 🤷🏻

Literally tried rg const_ops in the library folder and got nothing last time, now it worked, so, issue fixed I guess.

…ibutes explicitly

RalfJung · 2025-07-18T06:27:06Z

TBH I think it'd be fine to have arith and bit ops in the same feature gate. We don't need a dozen of them, and it seems likely we'd stabilize these together.

clarfonthey · 2025-07-18T12:49:01Z

That's fair. I could combine the bit ops I've already gotten working into this PR, I just figured that it made more sense to separate them.

Part of my reason was to just simplify the review, but I guess that the compiler is used to 100-file changes, so… 🤷🏻

I also thought there would be more implementations to constify but for the arithmetic side it's just Duration as the odd one out, and for bit operations it's Ipv[46]Addr. So, not groundbreaking.

clarfonthey · 2025-07-18T16:39:19Z

Okay, I went for it and just incorporated the bit operations in this PR and undid the rename. After this, "all" of the operators are constified.

I say "all" because I'm excluding the Index and Deref operators, which deserve separate handling.

blyxyas · 2025-07-18T22:37:36Z

src/tools/clippy/clippy_lints/src/operators/assign_op_pattern.rs

-                    && !is_stable_const_fn(cx, *binop_id, msrv)
-                {
+                // FIXME: reintroduce a better check after this is merged back into Clippy
+                if is_in_const_context(cx) {


Wouldn't we want to check for tcx.features().const_ops()? (I'm not being able to find const_ops on Features)

rustbot assigned ibraheemdev Jul 15, 2025

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the const-arith-ops branch from 97d85a1 to d7a8770 Compare July 15, 2025 02:31

This comment has been minimized.

Sign in to view

clarfonthey force-pushed the const-arith-ops branch from d7a8770 to ad7b7c4 Compare July 15, 2025 04:48

hkBst reviewed Jul 15, 2025

View reviewed changes

samueltardieu reviewed Jul 15, 2025

View reviewed changes

library/core/src/ops/arith.rs Show resolved Hide resolved

clarfonthey mentioned this pull request Jul 16, 2025

Custom handling for const-stability of assign operators in assign_op_pattern lint should be removed rust-lang/rust-clippy#15285

Open

clarfonthey force-pushed the const-arith-ops branch from ad7b7c4 to 071b5f0 Compare July 16, 2025 21:50

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-concerns Status: Awaiting concerns to be addressed by the author label Jul 16, 2025

samueltardieu mentioned this pull request Jul 16, 2025

Misleading error message when concern is left in a file comment rust-lang/triagebot#2114

Closed

rustbot removed the S-waiting-on-concerns Status: Awaiting concerns to be addressed by the author label Jul 17, 2025

clarfonthey force-pushed the const-arith-ops branch from 071b5f0 to ed0722b Compare July 17, 2025 11:59

This comment has been minimized.

Sign in to view

samueltardieu reviewed Jul 17, 2025

View reviewed changes

Let forward_ref_* macros accept multiple attributes, and require attr…

b0d60f1

…ibutes explicitly

clarfonthey force-pushed the const-arith-ops branch from ed0722b to 8e2f97b Compare July 18, 2025 00:04

clarfonthey added 2 commits July 18, 2025 12:35

Trivially constify remaining operators

f86b89c

Constify remaining impls

c4fe6d7

clarfonthey changed the title ~~Tidy up const_ops, rename to const_arith_ops~~ Constify remaining traits/impls for const_ops Jul 18, 2025

clarfonthey force-pushed the const-arith-ops branch from 8e2f97b to c4fe6d7 Compare July 18, 2025 16:41

blyxyas reviewed Jul 18, 2025

View reviewed changes

		#![feature(const_arith_ops)]
		#![feature(const_trait_impl)]

Constify remaining traits/impls for const_ops #143949

Are you sure you want to change the base?

Constify remaining traits/impls for const_ops #143949

Conversation

clarfonthey commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Concerns (0 active)

Uh oh!

rustbot commented Jul 15, 2025

Uh oh!

rustbot commented Jul 15, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

samueltardieu Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rustbot commented Jul 16, 2025

Uh oh!

This comment has been minimized.

samueltardieu commented Jul 16, 2025

Uh oh!

samueltardieu commented Jul 17, 2025

Uh oh!

This comment has been minimized.

clarfonthey commented Jul 17, 2025

Uh oh!

samueltardieu commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfonthey commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Jul 18, 2025

Uh oh!

clarfonthey commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clarfonthey commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Constify remaining traits/impls for `const_ops` #143949

Constify remaining traits/impls for `const_ops` #143949

clarfonthey commented Jul 15, 2025 •

edited

Loading

samueltardieu Jul 15, 2025 •

edited

Loading

clarfonthey Jul 17, 2025 •

edited

Loading

clarfonthey commented Jul 18, 2025 •

edited

Loading

clarfonthey commented Jul 18, 2025 •

edited

Loading